[gpt-oss][2/N] Support input_messages in responsesRequest#26962
[gpt-oss][2/N] Support input_messages in responsesRequest#26962yeqcharlotte merged 11 commits intovllm-project:mainfrom
Conversation
340b276 to
21815e5
Compare
|
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: Andrew Xia <axia@fb.com>
21815e5 to
ec420f3
Compare
Signed-off-by: Andrew Xia <axia@fb.com>
Signed-off-by: Andrew Xia <axia@fb.com>
Signed-off-by: Andrew Xia <axia@fb.com>
| get_system_message, | ||
| parse_chat_input, | ||
| parse_chat_output, | ||
| parse_input_to_harmony_message, |
There was a problem hiding this comment.
no functional changes in this file, just renamed the function and formatted code
Signed-off-by: Andrew Xia <axia@fb.com>
|
ready for review, |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for previous_input_messages in ResponsesRequest to enable multi-turn conversations, which is a valuable addition. The implementation is mostly sound and is accompanied by new tests. However, I've identified a couple of high-severity issues. One is a bug in the new test case where the tool name is incorrect, potentially masking issues. The other is a significant code duplication in the message processing logic, which impacts maintainability. Addressing these points will improve the robustness and quality of the code.
Signed-off-by: Andrew Xia <axia@fb.com>
04fad7a to
b1d2b02
Compare
qandrew
left a comment
There was a problem hiding this comment.
@yeqcharlotte @alecsolder thanks for the comments! should be ready for another review
| message_role = message.author.role | ||
| # To match OpenAI, instructions, reasoning and tools are | ||
| # always taken from the most recent Responses API request | ||
| # not carried over from previous requests | ||
| if ( | ||
| message_role == OpenAIHarmonyRole.SYSTEM | ||
| or message_role == OpenAIHarmonyRole.DEVELOPER | ||
| ): | ||
| continue |
There was a problem hiding this comment.
move these logic to harmony util?
There was a problem hiding this comment.
@yeqcharlotte thanks for the catch, done
yeqcharlotte
left a comment
There was a problem hiding this comment.
thanks for addressing all the updates. it looks much cleaner now!
…ct#26962) Signed-off-by: Andrew Xia <axia@fb.com> Co-authored-by: Andrew Xia <axia@fb.com>
…ct#26962) Signed-off-by: Andrew Xia <axia@fb.com> Co-authored-by: Andrew Xia <axia@fb.com>
…ct#26962) Signed-off-by: Andrew Xia <axia@fb.com> Co-authored-by: Andrew Xia <axia@fb.com>
…ct#26962) Signed-off-by: Andrew Xia <axia@fb.com> Co-authored-by: Andrew Xia <axia@fb.com>
Purpose
in the RepsonsesResponse object, we already have input_messages and output_messages. We should be able to do multi_turl by allowing ResponsesRequest to have previous_input_messages as payload.
Test Plan
I tested following OpenAI's example: https://platform.openai.com/docs/guides/function-calling#function-tool-example
Added the following as a test also.
Turn 1
Turn 1 output
Turn 2